Skip to content

Feat/hunspell cache api#20841

Closed
shayush622 wants to merge 11 commits intoopensearch-project:mainfrom
shayush622:feat/hunspell-cache-api
Closed

Feat/hunspell cache api#20841
shayush622 wants to merge 11 commits intoopensearch-project:mainfrom
shayush622:feat/hunspell-cache-api

Conversation

@shayush622
Copy link
Contributor

Description

This PR adds REST API endpoints for hunspell dictionary cache management. It introduces two endpoints for viewing and invalidating cached hunspell dictionaries.

Endpoints:

  • GET /_hunspell/cache — View all cached dictionary keys (requires cluster:monitor/hunspell/cache permission)
  • POST /_hunspell/cache/_invalidate — Invalidate by package_id, locale, cache_key, or invalidate_all
  • POST /_hunspell/cache/_invalidate_all — Invalidate all cached dictionaries

Key changes:

  • TransportHunspellCacheInfoAction and TransportHunspellCacheInvalidateAction
  • RestHunspellCacheInvalidateAction REST handler
  • Action registration in ActionModule
  • Consistent response schema with all fields always present
  • Comprehensive unit tests, REST handler tests, and integration test

This is Part 2 of 2 — depends on #20840 (ref_path core support).

Related Issues

Resolves #20712

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 0297edf.

'Diff too large, requires skip by maintainers after manual review'


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 05a74c4.

'Diff too large, requires skip by maintainers after manual review'


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@shayush622 shayush622 mentioned this pull request Mar 11, 2026
3 tasks
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces Hunspell dictionary cache management APIs (info + invalidation) and wires them through REST → transport actions → HunspellService, alongside related Hunspell package-based loading (ref_path) and cache-utility additions needed to support these endpoints.

Changes:

  • Add transport actions + REST handler for Hunspell cache info (GET /_hunspell/cache) and invalidation (POST /_hunspell/cache/_invalidate, /_invalidate_all).
  • Extend HunspellService/HunspellTokenFilterFactory with package-based dictionary loading and cache invalidation utilities.
  • Update node/module wiring and add unit + integration tests and test dictionary resources.

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
server/src/main/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateAction.java New REST handler for cache info + invalidation endpoints.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoAction.java New action type for cache info.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoRequest.java New transport request for cache info.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoResponse.java New transport/XContent response for cache info.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInfoAction.java Transport handler to read Hunspell cache state.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateAction.java New action type for cache invalidation.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateRequest.java New invalidation request + validation logic.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java New invalidation response with consistent schema fields.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInvalidateAction.java Transport handler performing invalidations via HunspellService.
server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/package-info.java New package docs for hunspell cache actions.
server/src/main/java/org/opensearch/action/ActionModule.java Registers new actions + REST handler; adds HunspellService binding.
server/src/main/java/org/opensearch/node/Node.java Passes HunspellService into ActionModule.
server/src/main/java/org/opensearch/indices/analysis/AnalysisModule.java Exposes getHunspellService() publicly for node wiring.
server/src/main/java/org/opensearch/indices/analysis/HunspellService.java Adds package dictionary loading + cache key utilities + invalidation APIs.
server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java Adds ref_path loading + validation + updateable analysis mode behavior.
server/src/internalClusterTest/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheIT.java Adds cluster-level tests for transport request/response + schema/validation.
server/src/test/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateActionTests.java Adds REST handler/unit tests for routes, request/response serialization, params.
server/src/test/java/org/opensearch/indices/analyze/HunspellServiceTests.java Adds unit tests for package dictionary loading + caching + invalidation.
server/src/test/java/org/opensearch/index/analysis/HunspellTokenFilterFactoryTests.java Adds tests for ref_path behavior, validation, and updateable mode.
server/src/test/java/org/opensearch/action/ActionModuleTests.java Updates constructor calls for new ActionModule signature.
server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java Updates constructor calls for new ActionModule signature.
server/src/test/resources/indices/analyze/conf_dir/packages/test-pkg/hunspell/en_US/en_US.aff Adds test hunspell affix file for package-based dictionary loading.
CHANGELOG.md Adds changelog entries for ref_path + cache API (currently with placeholder PR).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +42 to +59
/**
* Sets up hunspell dictionary files in the node's config directory.
* Creates both traditional and package-based dictionaries.
*/
private void setupDictionaries(Path configDir) throws IOException {
// Traditional dictionary: config/hunspell/en_US/
Path traditionalDir = configDir.resolve("hunspell").resolve("en_US");
Files.createDirectories(traditionalDir);
Files.write(traditionalDir.resolve("en_US.aff"), "SET UTF-8\n".getBytes(StandardCharsets.UTF_8));
Files.write(traditionalDir.resolve("en_US.dic"), "1\nhello\n".getBytes(StandardCharsets.UTF_8));

// Package-based dictionary: config/packages/test-pkg/hunspell/en_US/
Path packageDir = configDir.resolve("packages").resolve("test-pkg").resolve("hunspell").resolve("en_US");
Files.createDirectories(packageDir);
Files.write(packageDir.resolve("en_US.aff"), "SET UTF-8\n".getBytes(StandardCharsets.UTF_8));
Files.write(packageDir.resolve("en_US.dic"), "1\nworld\n".getBytes(StandardCharsets.UTF_8));
}

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setupDictionaries(...) is currently unused, and the integration tests never load a dictionary into the cache. As written, these tests will still pass even if cache-info/invalidation behavior for non-empty caches is broken. Please either remove the unused helper or use it to set up dictionaries and add assertions for non-empty cache counts/keys before and after invalidation.

Suggested change
/**
* Sets up hunspell dictionary files in the node's config directory.
* Creates both traditional and package-based dictionaries.
*/
private void setupDictionaries(Path configDir) throws IOException {
// Traditional dictionary: config/hunspell/en_US/
Path traditionalDir = configDir.resolve("hunspell").resolve("en_US");
Files.createDirectories(traditionalDir);
Files.write(traditionalDir.resolve("en_US.aff"), "SET UTF-8\n".getBytes(StandardCharsets.UTF_8));
Files.write(traditionalDir.resolve("en_US.dic"), "1\nhello\n".getBytes(StandardCharsets.UTF_8));
// Package-based dictionary: config/packages/test-pkg/hunspell/en_US/
Path packageDir = configDir.resolve("packages").resolve("test-pkg").resolve("hunspell").resolve("en_US");
Files.createDirectories(packageDir);
Files.write(packageDir.resolve("en_US.aff"), "SET UTF-8\n".getBytes(StandardCharsets.UTF_8));
Files.write(packageDir.resolve("en_US.dic"), "1\nworld\n".getBytes(StandardCharsets.UTF_8));
}

Copilot uses AI. Check for mistakes.
@cwperks cwperks added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Mar 11, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

PR Reviewer Guide 🔍

(Review updated until commit 99f2309)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: HunspellService package-based dictionary loading and ref_path support

Relevant files:

  • server/src/main/java/org/opensearch/indices/analysis/HunspellService.java
  • server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java
  • server/src/test/java/org/opensearch/indices/analyze/HunspellServiceTests.java
  • server/src/test/java/org/opensearch/index/analysis/HunspellTokenFilterFactoryTests.java

Sub-PR theme: Hunspell cache REST API endpoints and transport actions

Relevant files:

  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoAction.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoRequest.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInfoResponse.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateAction.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateRequest.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInfoAction.java
  • server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInvalidateAction.java
  • server/src/main/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateAction.java
  • server/src/main/java/org/opensearch/action/ActionModule.java
  • server/src/main/java/org/opensearch/node/Node.java
  • server/src/test/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateActionTests.java
  • server/src/internalClusterTest/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheIT.java

⚡ Recommended focus areas for review

Null Binding Risk

When hunspellService is null, the Guice binding is skipped entirely. This means TransportHunspellCacheInfoAction and TransportHunspellCacheInvalidateAction will fail with an injection error at runtime rather than a clear error message. Consider binding a no-op or throwing a more descriptive exception, or ensuring the transport actions handle the missing binding gracefully.

if (hunspellService != null) {
    bind(HunspellService.class).toInstance(hunspellService);
} else {
    logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
}
Race Condition

In invalidateDictionariesByPackage, the count is computed as sizeBefore - dictionaries.size() after removeIf. Since dictionaries is a ConcurrentHashMap, concurrent modifications between the two size reads can produce an inaccurate or even negative count. The Javadoc acknowledges this, but callers receiving a negative count could be confusing. Consider using an AtomicInteger counter inside the removeIf predicate for accuracy.

public int invalidateDictionariesByPackage(String packageId) {
    if (Strings.isNullOrEmpty(packageId)) {
        logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
        return 0;
    }

    String prefix = packageId + CACHE_KEY_SEPARATOR;
    int sizeBefore = dictionaries.size();
    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
    int count = sizeBefore - dictionaries.size();

    if (count > 0) {
        logger.info("Invalidated {} hunspell dictionary cache entries for package: {}", count, packageId);
    } else {
        logger.debug("No cached dictionaries found for package: {}", packageId);
    }
    return count;
}
Path-based Routing

The buildInvalidateRequest method uses request.path().endsWith("/_invalidate_all") to distinguish the invalidate-all route. This is fragile — if the path contains a trailing slash or query string, the check may fail. Consider using a dedicated route parameter or comparing the matched route pattern instead.

if (request.path().endsWith("/_invalidate_all")) {
    invalidateRequest.setInvalidateAll(true);
} else {
    String packageId = request.param("package_id");
    String locale = request.param("locale");
    String cacheKey = request.param("cache_key");

    invalidateRequest.setPackageId(packageId);
    invalidateRequest.setLocale(locale);
    invalidateRequest.setCacheKey(cacheKey);
}
Single-node Only

This transport action extends HandledTransportAction and only executes on the coordinating node. In a multi-node cluster, each node has its own HunspellService cache. The invalidation will only affect the node that receives the request, leaving other nodes' caches stale. Consider using a broadcast/nodes action (e.g., extending TransportNodesAction) to invalidate across all nodes.

public class TransportHunspellCacheInvalidateAction extends HandledTransportAction<
    HunspellCacheInvalidateRequest,
    HunspellCacheInvalidateResponse> {

    private final HunspellService hunspellService;

    @Inject
    public TransportHunspellCacheInvalidateAction(
        TransportService transportService,
        ActionFilters actionFilters,
        HunspellService hunspellService
    ) {
        super(HunspellCacheInvalidateAction.NAME, transportService, actionFilters, HunspellCacheInvalidateRequest::new);
        this.hunspellService = hunspellService;
    }

    @Override
    protected void doExecute(Task task, HunspellCacheInvalidateRequest request, ActionListener<HunspellCacheInvalidateResponse> listener) {
        try {
            String packageId = request.getPackageId();
            String locale = request.getLocale();
            String cacheKey = request.getCacheKey();
            boolean invalidateAll = request.isInvalidateAll();

            int invalidatedCount = 0;
            String responseCacheKey = null;

            if (invalidateAll) {
                // Invalidate all cached dictionaries
                invalidatedCount = hunspellService.invalidateAllDictionaries();
            } else if (packageId != null && locale != null) {
                // Invalidate specific package + locale combination
                responseCacheKey = HunspellService.buildPackageCacheKey(packageId, locale);
                boolean invalidated = hunspellService.invalidateDictionary(responseCacheKey);
                invalidatedCount = invalidated ? 1 : 0;
            } else if (packageId != null) {
                // Invalidate all locales for a package
                invalidatedCount = hunspellService.invalidateDictionariesByPackage(packageId);
            } else if (cacheKey != null) {
                // Invalidate a specific cache key directly
                responseCacheKey = cacheKey;
                boolean invalidated = hunspellService.invalidateDictionary(cacheKey);
                invalidatedCount = invalidated ? 1 : 0;
            }

            listener.onResponse(new HunspellCacheInvalidateResponse(true, invalidatedCount, packageId, locale, responseCacheKey));
        } catch (Exception e) {
            listener.onFailure(e);
        }
    }
Locale Validation Change

The new validatePackageIdentifier is now also applied to traditional locale values (e.g., "en_US"). The regex ^[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$ rejects single-character locales with underscores or hyphens, and may reject valid locale strings that were previously accepted (e.g., locales with special characters). This is a potentially breaking change for existing users.

validatePackageIdentifier(locale, "locale");
dictionary = hunspellService.getDictionary(locale);

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

PR Code Suggestions ✨

Latest suggestions up to 99f2309

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix inaccurate concurrent removal count

The invalidateDictionariesByPackage method uses a non-atomic size-before/size-after
approach to count removed entries on a ConcurrentHashMap, which can yield inaccurate
counts under concurrent modifications. Instead, use an AtomicInteger counter
incremented inside the removeIf predicate to accurately count actual removals.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [481-484]

 public int invalidateDictionariesByPackage(String packageId) {
-    ...
+    if (Strings.isNullOrEmpty(packageId)) {
+        logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
+        return 0;
+    }
+
     String prefix = packageId + CACHE_KEY_SEPARATOR;
-    int sizeBefore = dictionaries.size();
-    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
-    int count = sizeBefore - dictionaries.size();
+    java.util.concurrent.atomic.AtomicInteger count = new java.util.concurrent.atomic.AtomicInteger(0);
+    dictionaries.keySet().removeIf(key -> {
+        if (key.startsWith(prefix)) {
+            count.incrementAndGet();
+            return true;
+        }
+        return false;
+    });
+    int removed = count.get();
Suggestion importance[1-10]: 6

__

Why: The non-atomic size-before/size-after approach on a ConcurrentHashMap can yield inaccurate counts under concurrent modifications. Using an AtomicInteger inside the removeIf predicate is a valid improvement, though the method's Javadoc already acknowledges the approximate nature of the count.

Low
General
Validate REST request parameters before dispatching

The buildInvalidateRequest method does not validate the request before dispatching
it to the transport layer. The HunspellCacheInvalidateRequest.validate() method can
detect invalid parameter combinations (e.g., no parameters set, conflicting
parameters), but it is never called in the REST handler, so invalid requests will
reach the transport action without early rejection.

server/src/main/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateAction.java [106-116]

 if (request.path().endsWith("/_invalidate_all")) {
     invalidateRequest.setInvalidateAll(true);
 } else {
     String packageId = request.param("package_id");
     String locale = request.param("locale");
     String cacheKey = request.param("cache_key");
 
     invalidateRequest.setPackageId(packageId);
     invalidateRequest.setLocale(locale);
     invalidateRequest.setCacheKey(cacheKey);
 }
 
+ActionRequestValidationException validationException = invalidateRequest.validate();
+if (validationException != null) {
+    throw new IllegalArgumentException(validationException.getMessage());
+}
+
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that validate() is never called in the REST handler, meaning invalid requests reach the transport layer. However, OpenSearch's transport framework typically calls validate() automatically before executing the action, so this may be redundant, reducing the impact.

Low
Prevent concurrent dictionary load conflicts in temp dir

The loadDictionaryFromDirectory method uses env.tmpDir() for the Lucene
NIOFSDirectory used during dictionary loading. This is a shared temporary directory,
and concurrent dictionary loads could potentially conflict. Consider using a unique
subdirectory per load (e.g., using a UUID or the description) to avoid conflicts
under concurrent loading.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [388-390]

-try (Directory tmp = new NIOFSDirectory(env.tmpDir())) {
+Path tmpDir = env.tmpDir().resolve("hunspell-" + java.util.UUID.randomUUID());
+java.nio.file.Files.createDirectories(tmpDir);
+try (Directory tmp = new NIOFSDirectory(tmpDir)) {
     return new Dictionary(tmp, "hunspell", affixStream, dicStreams, ignoreCase);
+} finally {
+    IOUtils.rm(tmpDir);
 }
Suggestion importance[1-10]: 5

__

Why: Using a shared env.tmpDir() for concurrent dictionary loads could cause conflicts. The suggestion to use a unique subdirectory per load is valid, but the computeIfAbsent on ConcurrentHashMap already provides some serialization guarantees for the same key, and the Lucene Dictionary constructor uses the directory as a temporary workspace that may not conflict in practice.

Low
Prevent opaque injection failure when service is null

When hunspellService is null, the TransportHunspellCacheInfoAction and
TransportHunspellCacheInvalidateAction classes will fail at injection time with an
opaque Guice error rather than a clear message. Consider binding a no-op or throwing
a more descriptive exception, or ensuring the transport actions handle a
null/missing binding gracefully.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/ActionModule.java [1131-1135]

 if (hunspellService != null) {
     bind(HunspellService.class).toInstance(hunspellService);
 } else {
     logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
+    bind(HunspellService.class).toProvider(() -> {
+        throw new IllegalStateException("HunspellService is not available; hunspell cache management APIs are not functional");
+    });
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion addresses a real concern about opaque Guice injection failures when hunspellService is null, but the relevant_file in the suggestion incorrectly references ActionModule.java under the wrong package path. The improvement is valid but the impact is limited since HunspellService should always be available in production.

Low
Add missing newline at end of file

The dictionary file is missing a newline at the end of the file, as indicated by the
\ No newline at end of file warning. Dictionary files should end with a newline to
comply with POSIX standards and avoid potential parsing issues with some tools.

server/src/test/resources/indices/analyze/conf_dir/packages/test-pkg/hunspell/en_US/en_US.dic [106]

-+secure
+secure
Suggestion importance[1-10]: 3

__

Why: The suggestion about a missing newline at end of file is valid for POSIX compliance, but the diff doesn't explicitly show a "No newline at end of file" warning. The improved_code is essentially the same as existing_code (just secure without the + prefix), making the actual change unclear. This is a minor style/standards issue with low impact.

Low

Previous suggestions

Suggestions up to commit 13ba03f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix dictionary word count mismatch

The dictionary file declares 100 words in its header count, but the actual number of
word entries in the file is 105 (lines 2–106). The header count must match the
actual number of entries, or Hunspell will fail to load all words correctly.

server/src/test/resources/indices/analyze/conf_dir/packages/test-pkg/hunspell/en_US/en_US.dic [1]

-100
+105
Suggestion importance[1-10]: 8

__

Why: The header count 100 doesn't match the actual 105 word entries (lines 2-106). In Hunspell format, the first line must accurately declare the number of entries, or the dictionary may not load all words correctly.

Medium
Eliminate race condition in dictionary reload

The reloadDictionaryFromPackage method is not atomic: between invalidateDictionary
and getDictionaryFromPackage, another thread could load the old or a
partially-loaded dictionary into the cache. Use dictionaries.compute or
dictionaries.merge to atomically replace the entry, ensuring no window exists where
the cache is empty and another thread races to load.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [521-525]

 public Dictionary reloadDictionaryFromPackage(String packageId, String locale) {
     String cacheKey = buildPackageCacheKey(packageId, locale);
-    invalidateDictionary(cacheKey);
-    return getDictionaryFromPackage(packageId, locale);
+    try {
+        Dictionary fresh = loadDictionaryFromPackage(packageId, locale);
+        dictionaries.put(cacheKey, fresh);
+        logger.info("Reloaded hunspell dictionary for package [{}] locale [{}]", packageId, locale);
+        return fresh;
+    } catch (Exception e) {
+        throw new IllegalStateException(
+            String.format(Locale.ROOT, "Failed to reload hunspell dictionary for package [%s] locale [%s]", packageId, locale),
+            e
+        );
+    }
 }
Suggestion importance[1-10]: 7

__

Why: The two-step invalidate-then-load in reloadDictionaryFromPackage has a real race window where concurrent threads could load a stale or partial dictionary. The improved code atomically loads and replaces the entry, which is a meaningful correctness improvement for concurrent usage.

Medium
Fix inaccurate concurrent count in bulk invalidation

The dictionaries field is a ConcurrentHashMap, but the pattern of reading size(),
calling removeIf, then computing the difference is not atomic. Under concurrent
access, the reported count can be inaccurate or negative. Consider using an
AtomicInteger counter incremented inside the removeIf predicate to accurately count
removed entries.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [481-484]

 public int invalidateDictionariesByPackage(String packageId) {
-    ...
+    if (Strings.isNullOrEmpty(packageId)) {
+        logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
+        return 0;
+    }
+
     String prefix = packageId + CACHE_KEY_SEPARATOR;
-    int sizeBefore = dictionaries.size();
-    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
-    int count = sizeBefore - dictionaries.size();
+    java.util.concurrent.atomic.AtomicInteger count = new java.util.concurrent.atomic.AtomicInteger(0);
+    dictionaries.keySet().removeIf(key -> {
+        if (key.startsWith(prefix)) {
+            count.incrementAndGet();
+            return true;
+        }
+        return false;
+    });
+    int removed = count.get();
+    if (removed > 0) {
+        logger.info("Invalidated {} hunspell dictionary cache entries for package: {}", removed, packageId);
+    } else {
+        logger.debug("No cached dictionaries found for package: {}", packageId);
+    }
+    return removed;
+}
Suggestion importance[1-10]: 6

__

Why: The size() + removeIf + size() pattern on a ConcurrentHashMap is not atomic and can yield inaccurate counts under concurrent access. Using an AtomicInteger inside the predicate is a valid fix, though the comment in the code already acknowledges this approximation is acceptable for diagnostic purposes.

Low
Prevent silent null binding causing runtime injection failure

When hunspellService is null, the TransportHunspellCacheInfoAction and
TransportHunspellCacheInvalidateAction classes are registered as transport actions
but HunspellService is not bound in Guice. This will cause an injection failure at
runtime when those actions are invoked, rather than a clean startup warning. Either
throw an exception at startup or provide a no-op binding to prevent a confusing
runtime error.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/ActionModule.java [1131-1135]

 if (hunspellService != null) {
     bind(HunspellService.class).toInstance(hunspellService);
 } else {
-    logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
+    throw new IllegalStateException(
+        "HunspellService is null; hunspell cache management APIs cannot be initialized. " +
+        "Ensure AnalysisModule is properly configured."
+    );
 }
Suggestion importance[1-10]: 5

__

Why: When hunspellService is null, the transport actions are registered but HunspellService is unbound in Guice, which would cause a runtime injection failure. However, the suggestion references the wrong file (ActionModule.java is in action package, not admin/indices/cache/hunspell), and the code comment already explains this is a defensive guard for testing scenarios where null is intentionally passed.

Low
General
Validate REST request before dispatching to transport layer

The buildInvalidateRequest method does not validate the resulting request before
dispatching it. If no parameters are provided for the /_invalidate path, the request
will fail at the transport layer with a generic validation error. It is better to
call request.validate() here and throw an IllegalArgumentException with a clear
message so the REST layer returns a proper 400 response.

server/src/main/java/org/opensearch/rest/action/admin/indices/RestHunspellCacheInvalidateAction.java [106-116]

 if (request.path().endsWith("/_invalidate_all")) {
     invalidateRequest.setInvalidateAll(true);
 } else {
     String packageId = request.param("package_id");
     String locale = request.param("locale");
     String cacheKey = request.param("cache_key");
 
     invalidateRequest.setPackageId(packageId);
     invalidateRequest.setLocale(locale);
     invalidateRequest.setCacheKey(cacheKey);
 }
 
+ActionRequestValidationException validationException = invalidateRequest.validate();
+if (validationException != null) {
+    throw new IllegalArgumentException(validationException.getMessage());
+}
+
Suggestion importance[1-10]: 4

__

Why: Adding early validation in the REST layer provides a cleaner 400 response, but OpenSearch's transport layer already handles ActionRequestValidationException and returns appropriate error responses, making this improvement minor.

Low
Suggestions up to commit da51569
CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore missing version guard for expandedDistDir assignment

The original constructor had a guard version.onOrAfter("7.10.0") before setting
expandedDistDir, but the new constructor unconditionally sets expandedDistDir for
zip/tar without this version check. This removes a previously intentional version
constraint and may cause incorrect behavior for older BWC versions below 7.10.0.

buildSrc/src/main/java/org/opensearch/gradle/internal/InternalDistributionBwcSetupPlugin.java [307-311]

-// we only ported this down to the 7.x branch.
-if (version.onOrAfter("7.10.0") && (name.endsWith("zip") || name.endsWith("tar"))) {
-    this.expandedDistDir = new File(checkoutDir, baseDir + "/" + name + "/build/install");
+if (name.endsWith("zip") || name.endsWith("tar")) {
+    if (expectedVersion.onOrAfter("7.10.0")) {
+        this.expandedDistDir = new File(checkoutDir, baseDir + "/" + name + "/build/install");
+    } else {
+        this.expandedDistDir = null;
+    }
+} else {
+    this.expandedDistDir = null;
 }
Suggestion importance[1-10]: 7

__

Why: The original code had an explicit version.onOrAfter("7.10.0") guard before setting expandedDistDir, but the new constructor removes this check. This could cause incorrect behavior for BWC versions below 7.10.0, and the fix is straightforward and important for correctness.

Medium
Restore correct deduplication of tail log messages

The original code used errorsAndWarnings.containsKey(normalizeLogLine(message)) to
skip messages already shown in the errors/warnings section. The new code filters by
!message.contains("ERROR") && !message.contains("WARN"), which is a different (and
potentially incorrect) heuristic — it would suppress any tail message containing
those strings even if it wasn't in errorsAndWarnings, and conversely would show
messages that were added to errorsAndWarnings via the normalized key if they don't
literally contain "ERROR"/"WARN". This changes the deduplication behavior.

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java [1060-1062]

 ring.stream()
-    .filter(message -> !message.contains("ERROR") && !message.contains("WARN"))
+    .filter(message -> {
+        String normalized = normalizeLogLine(message);
+        return errorsAndWarnings.keySet().stream().noneMatch(lm -> lm.normalizedMessage().equals(normalized));
+    })
     .forEach(message -> LOGGER.lifecycle("» " + message.replace("\n", "\n»  ")));
Suggestion importance[1-10]: 7

__

Why: The new filter !message.contains("ERROR") && !message.contains("WARN") is a behavioral regression from the original logic that used errorsAndWarnings.containsKey(normalizeLogLine(message)). The original correctly deduplicated messages already shown in the errors section, while the new approach may show duplicates or suppress valid tail messages. The improved code restores the correct deduplication behavior.

Medium
Guard against division by zero for virtual shards

When numVirtualShards is 0, Math.floorMod(hash, 0) will throw an ArithmeticException
(division by zero). A guard check should be added to ensure numVirtualShards is a
positive value before performing the modulo operation.

server/src/main/java/org/opensearch/cluster/routing/OperationRouting.java [525-530]

 int numVirtualShards = indexMetadata.getNumberOfVirtualShards();
-if (numVirtualShards != -1) {
+if (numVirtualShards > 0) {
     final int hash = Murmur3HashFunction.hash(effectiveRouting) + partitionOffset;
     int vShardId = Math.floorMod(hash, numVirtualShards);
     return VirtualShardRoutingHelper.resolvePhysicalShardId(indexMetadata, vShardId);
 }
Suggestion importance[1-10]: 7

__

Why: If getNumberOfVirtualShards() can return 0, Math.floorMod(hash, 0) would throw an ArithmeticException. Changing the condition from != -1 to > 0 is a simple and correct fix that prevents a potential runtime crash.

Medium
Avoid throwing inside computeIfAbsent on ConcurrentHashMap

computeIfAbsent does not handle exceptions thrown by the mapping function gracefully
— if the function throws, the entry is not cached, but the exception propagates.
However, a more critical issue is that if a RuntimeException is thrown inside
computeIfAbsent, the ConcurrentHashMap may leave the map in an inconsistent state
under concurrent access in some JVM versions. Consider computing the value outside
of computeIfAbsent or using a putIfAbsent pattern with explicit error handling to
avoid this.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [180-191]

-return dictionaries.computeIfAbsent(cacheKey, (key) -> {
-    try {
-        return loadDictionaryFromPackage(packageId, locale);
-    } catch (Exception e) {
+Dictionary existing = dictionaries.get(cacheKey);
+if (existing != null) {
+    return existing;
+}
+try {
+    Dictionary loaded = loadDictionaryFromPackage(packageId, locale);
+    Dictionary prev = dictionaries.putIfAbsent(cacheKey, loaded);
+    return prev != null ? prev : loaded;
+} catch (Exception e) {
+    throw new IllegalStateException(
+        String.format(Locale.ROOT, "Failed to load hunspell dictionary for package [%s] locale [%s]", packageId, locale),
+        e
+    );
+}
 
-        throw new IllegalStateException(
-            String.format(Locale.ROOT, "Failed to load hunspell dictionary for package [%s] locale [%s]", packageId, locale),
-            e
-        );
-    }
-});
-
Suggestion importance[1-10]: 6

__

Why: Throwing a RuntimeException inside ConcurrentHashMap.computeIfAbsent can leave the map in an inconsistent state in some JVM versions. The suggested get/putIfAbsent pattern is safer, though the practical impact depends on the JVM version and concurrency level.

Low
Validate request before processing in transport action

The doExecute method does not call request.validate() before processing. If the
request arrives with conflicting or missing parameters (e.g., both invalidateAll and
packageId set), the action will silently process an invalid state rather than
returning a validation error. The transport action should validate the request and
fail fast with a meaningful error.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInvalidateAction.java [45-77]

 @Override
 protected void doExecute(Task task, HunspellCacheInvalidateRequest request, ActionListener<HunspellCacheInvalidateResponse> listener) {
+    ActionRequestValidationException validationException = request.validate();
+    if (validationException != null) {
+        listener.onFailure(validationException);
+        return;
+    }
     try {
         ...
     } catch (Exception e) {
         listener.onFailure(e);
     }
 }
Suggestion importance[1-10]: 5

__

Why: The transport action skips calling request.validate(), which means conflicting or invalid parameter combinations could be processed silently. While the REST layer may validate before sending, transport actions should be defensive. The suggestion is valid but the existing_code snippet uses ... which doesn't match the actual diff exactly.

Low
Security
Fix pattern to reliably prevent consecutive dots

The SAFE_IDENTIFIER_PATTERN allows dots in identifiers, but the subsequent .. check
is a separate step that could be bypassed if the pattern is changed. More
critically, the pattern allows single-character values (via the second alternative
^[a-zA-Z0-9]$) but the .. check would never trigger for them anyway. The real issue
is that the .. check is redundant with the pattern — the pattern already requires
that dots are surrounded by alphanumeric characters or other valid chars, so
foo..bar would still match. Verify that SAFE_IDENTIFIER_PATTERN actually rejects ..
sequences, or remove the redundant check and rely solely on the pattern.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [152]

-private static final Pattern SAFE_IDENTIFIER_PATTERN = Pattern.compile("^[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$");
+private static final Pattern SAFE_IDENTIFIER_PATTERN = Pattern.compile("^[a-zA-Z0-9][a-zA-Z0-9_-]*(?:\\.[a-zA-Z0-9][a-zA-Z0-9_-]*)*$|^[a-zA-Z0-9]$");
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the current SAFE_IDENTIFIER_PATTERN allows foo..bar to match (since . is in the character class [a-zA-Z0-9._-]*), making the subsequent .. check the only guard. The improved pattern enforces that dots must be surrounded by non-dot characters, making the validation more robust. However, the separate .. check already handles this case, so the risk is moderate.

Low
Avoid unbounded memory allocation from stream input

Reading an untrusted rowCount from the stream and directly using it as the initial
capacity of ArrayList could cause excessive memory allocation if a malicious or
corrupted stream sends a very large value. Consider capping the initial capacity to
a reasonable maximum or using a default capacity and letting the list grow
naturally.

sandbox/plugins/analytics-engine/src/test/java/org/opensearch/ppl/action/PPLResponse.java [33-46]

 public PPLResponse(StreamInput in) throws IOException {
     super(in);
     this.columns = in.readStringList();
     int rowCount = in.readVInt();
-    this.rows = new ArrayList<>(rowCount);
+    this.rows = new ArrayList<>();
     for (int i = 0; i < rowCount; i++) {
         int colCount = in.readVInt();
         Object[] row = new Object[colCount];
         for (int j = 0; j < colCount; j++) {
             row[j] = in.readGenericValue();
         }
         rows.add(row);
     }
 }
Suggestion importance[1-10]: 3

__

Why: This is a sandbox/test file, so the security concern is lower priority. Additionally, OpenSearch's StreamInput typically has built-in protections against malicious input, and the colCount allocation for Object[] has the same issue but isn't addressed in the suggestion.

Low
General
Fix inaccurate concurrent removal count tracking

The count calculation using sizeBefore - dictionaries.size() is not thread-safe on a
ConcurrentHashMap — concurrent additions or removals between the size() call and the
removeIf call can produce an inaccurate or even negative count. While the Javadoc
acknowledges approximate counts, a negative result could be misleading. Consider
tracking removals directly within the removeIf predicate using an AtomicInteger for
a more accurate count.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [475-492]

-public int invalidateDictionariesByPackage(String packageId) {
-    if (Strings.isNullOrEmpty(packageId)) {
-        logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
-        return 0;
+String prefix = packageId + CACHE_KEY_SEPARATOR;
+java.util.concurrent.atomic.AtomicInteger count = new java.util.concurrent.atomic.AtomicInteger(0);
+dictionaries.keySet().removeIf(key -> {
+    if (key.startsWith(prefix)) {
+        count.incrementAndGet();
+        return true;
     }
+    return false;
+});
+int removed = count.get();
+if (removed > 0) {
+    logger.info("Invalidated {} hunspell dictionary cache entries for package: {}", removed, packageId);
+} else {
+    logger.debug("No cached dictionaries found for package: {}", packageId);
+}
+return removed;
 
-    String prefix = packageId + CACHE_KEY_SEPARATOR;
-    int sizeBefore = dictionaries.size();
-    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
-    int count = sizeBefore - dictionaries.size();
-
Suggestion importance[1-10]: 5

__

Why: The sizeBefore - dictionaries.size() approach is indeed not thread-safe and could yield a negative count under concurrent modifications. Using an AtomicInteger inside removeIf is a cleaner and more accurate approach, though the Javadoc already acknowledges approximate counts.

Low
Restrict rule pattern to boundary node subtrees

Using any() as the operand pattern means this rule will match a LogicalAggregate on
top of any relation, including non-boundary nodes. This could cause the rule to fire
prematurely before a boundary node exists in the subtree, and
AbsorbRuleUtils.findBoundary would return null and silently skip. The pattern should
be more specific to only match when an OpenSearchBoundaryTableScan is reachable, or
at minimum the rule description should document this behavior.

sandbox/plugins/analytics-engine/src/test/java/org/opensearch/ppl/planner/rules/AbsorbAggregateRule.java [32-35]

 private AbsorbAggregateRule(SqlOperatorTable operatorTable) {
-    super(operand(LogicalAggregate.class, any()), "AbsorbAggregateRule");
+    super(operand(LogicalAggregate.class, operand(OpenSearchBoundaryTableScan.class, any())), "AbsorbAggregateRule");
     this.operatorTable = operatorTable;
 }
Suggestion importance[1-10]: 5

__

Why: Using any() allows the rule to fire on aggregates without a boundary node in the subtree, which is handled by the null check on findBoundary. However, the suggested pattern operand(LogicalAggregate.class, operand(OpenSearchBoundaryTableScan.class, any())) would only match direct parent-child relationships, potentially missing cases with intermediate nodes between the aggregate and boundary scan.

Low
Use consistent VInt encoding for non-negative counts

The writeTo method uses out.writeInt(invalidatedCount) but the
HunspellCacheInfoResponse uses readVInt/writeVInt for its counts. For consistency
and wire-format correctness, invalidatedCount should use writeVInt/readVInt since it
represents a non-negative count. Using readInt here while writing with writeInt is
consistent within this class, but if the intent is a non-negative count, VInt is
more appropriate and matches the sibling response class.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java [43]

-this.invalidatedCount = in.readInt();
+this.invalidatedCount = in.readVInt();
Suggestion importance[1-10]: 4

__

Why: The writeTo uses writeInt and readInt consistently within this class, so there's no wire-format bug. The suggestion is a style/consistency improvement to match HunspellCacheInfoResponse, but it's not a correctness issue since both sides of the serialization are consistent.

Low
Use consistent total count after partitioning

The cachedKeys.size() passed to the response is computed before partitioning, but
packageKeys.size() + localeKeys.size() should always equal cachedKeys.size().
However, if getCachedDictionaryKeys() returns a live/mutable view of the cache, the
set could change between the size call and the iteration, leading to inconsistent
counts. Capture the total size after partitioning to ensure consistency.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/TransportHunspellCacheInfoAction.java [46-65]

 Set<String> cachedKeys = hunspellService.getCachedDictionaryKeys();
 
 Set<String> packageKeys = new HashSet<>();
 Set<String> localeKeys = new HashSet<>();
 
 for (String key : cachedKeys) {
     if (HunspellService.isPackageCacheKey(key)) {
         packageKeys.add(key);
     } else {
         localeKeys.add(key);
     }
 }
 
+int totalSize = packageKeys.size() + localeKeys.size();
 HunspellCacheInfoResponse response = new HunspellCacheInfoResponse(
-    cachedKeys.size(),
+    totalSize,
     packageKeys.size(),
     localeKeys.size(),
     packageKeys,
     localeKeys
 );
Suggestion importance[1-10]: 4

__

Why: The concern about a live/mutable view is valid but speculative without knowing the implementation of getCachedDictionaryKeys(). If it returns a snapshot (e.g., a new HashSet), the issue doesn't exist. The fix is reasonable but low-impact in practice.

Low
Verify shared environment usage in refactored method

The loadDictionaryFromDirectory method uses env.tmpDir() which is the instance field
env stored in the class. However, this method is also called from the original
loadDictionary path which previously used the env parameter passed to that method.
Verify that the stored this.env is always the correct environment to use, especially
since the original loadDictionary method accepted env as a parameter — if callers
could pass a different environment, this refactoring introduces a subtle bug.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [368-395]

 private Dictionary loadDictionaryFromDirectory(Path dicDir, boolean ignoreCase, String description) throws Exception {
+    // Uses this.env (stored at construction time) for tmpDir
+    Path[] affixFiles = FileSystemUtils.files(dicDir, "*.aff");
     ...
-    try (Directory tmp = new NIOFSDirectory(env.tmpDir())) {
+    try (Directory tmp = new NIOFSDirectory(this.env.tmpDir())) {
         return new Dictionary(tmp, "hunspell", affixStream, dicStreams, ignoreCase);
     }
Suggestion importance[1-10]: 3

__

Why: The suggestion asks to verify that this.env is always correct after refactoring, but looking at the PR code, env is stored as an instance field in the constructor and the original loadDictionary method also used the same env parameter passed to the constructor. The 'improved_code' is essentially the same as the existing code with just a comment added, making this a low-impact verification suggestion.

Low
Suggestions up to commit 04f3e4b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix mismatched integer read/write methods

The invalidatedCount is written with out.writeInt but read with in.readInt, while
the corresponding HunspellCacheInfoResponse uses readVInt/writeVInt. Since
invalidatedCount is always non-negative, it should use writeVInt/readVInt for
consistency and wire compatibility. Mixing writeInt and readVInt would cause a
deserialization bug if the serialization is ever changed.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java [40-47]

 public HunspellCacheInvalidateResponse(StreamInput in) throws IOException {
     super(in);
     this.acknowledged = in.readBoolean();
-    this.invalidatedCount = in.readInt();
+    this.invalidatedCount = in.readVInt();
     this.packageId = in.readOptionalString();
     this.locale = in.readOptionalString();
     this.cacheKey = in.readOptionalString();
 }
Suggestion importance[1-10]: 7

__

Why: The invalidatedCount is written with writeInt but read with readInt, while the info response uses readVInt/writeVInt. This inconsistency should be fixed for correctness and consistency. The improved code accurately changes readInt to readVInt, which must be paired with the corresponding writeVInt fix in writeTo.

Medium
Prevent injection failure when HunspellService is null

When hunspellService is null, the TransportHunspellCacheInfoAction and
TransportHunspellCacheInvalidateAction classes are registered as action handlers but
HunspellService is not bound in Guice. This will cause an injection failure at
runtime when those transport actions are instantiated, rather than a graceful
degradation. Either always bind a non-null HunspellService, or avoid registering the
transport actions when the service is null.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/ActionModule.java [1131-1135]

 if (hunspellService != null) {
     bind(HunspellService.class).toInstance(hunspellService);
 } else {
     logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
+    // Bind a no-op or throw to prevent injection failure for registered transport actions
+    throw new IllegalStateException("HunspellService must not be null when hunspell cache actions are registered");
 }
Suggestion importance[1-10]: 7

__

Why: When hunspellService is null, the transport actions are still registered but HunspellService is not bound in Guice, which will cause a runtime injection failure rather than graceful degradation. The suggestion correctly identifies this issue, though the improved code's approach of throwing an exception may be too aggressive — a better fix might be to conditionally skip registering the actions. The relevant file path in the suggestion is incorrect (ActionModule.java not cache/hunspell/ActionModule.java), but the code location is correct.

Medium
Fix inconsistent integer serialization for count field

The toXContent method serializes null fields as JSON null values, which may expose
internal state unnecessarily. Consider using builder.field(...) only when the value
is non-null, or use builder.nullableField(...) intentionally. More critically, the
invalidatedCount field is written with writeInt but read with readInt, while
totalCachedCount in the info response uses writeVInt/readVInt — this inconsistency
could cause deserialization issues if counts are large, but more importantly the
invalidatedCount should use readVInt/writeVInt for consistency with non-negative
counts.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java [49-56]

 @Override
-public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-    builder.startObject();
-    builder.field("acknowledged", acknowledged);
-    builder.field("invalidated_count", invalidatedCount);
-    builder.field("package_id", packageId);
-    builder.field("locale", locale);
-    builder.field("cache_key", cacheKey);
-    builder.endObject();
-    return builder;
+public void writeTo(StreamOutput out) throws IOException {
+    out.writeBoolean(acknowledged);
+    out.writeVInt(invalidatedCount);
+    out.writeOptionalString(packageId);
+    out.writeOptionalString(locale);
+    out.writeOptionalString(cacheKey);
 }
Suggestion importance[1-10]: 6

__

Why: The writeTo method uses out.writeInt for invalidatedCount while HunspellCacheInfoResponse uses writeVInt. Since invalidatedCount is always non-negative, using writeVInt/readVInt would be more consistent and efficient. The suggestion correctly identifies the inconsistency and the improved code accurately reflects the fix.

Low
General
Fix non-atomic count calculation in concurrent cache invalidation

The count calculation using sizeBefore - dictionaries.size() is not atomic on a
ConcurrentHashMap — concurrent additions or removals between the size() call and
removeIf can produce an incorrect (even negative) count. While the Javadoc
acknowledges approximate counts, a negative count could be misleading. Consider
counting removed entries directly within the removeIf predicate using an
AtomicInteger.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [475-492]

 public int invalidateDictionariesByPackage(String packageId) {
     if (Strings.isNullOrEmpty(packageId)) {
         logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
         return 0;
     }
 
     String prefix = packageId + CACHE_KEY_SEPARATOR;
-    int sizeBefore = dictionaries.size();
-    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
-    int count = sizeBefore - dictionaries.size();
-    ...
+    java.util.concurrent.atomic.AtomicInteger count = new java.util.concurrent.atomic.AtomicInteger(0);
+    dictionaries.keySet().removeIf(key -> {
+        if (key.startsWith(prefix)) {
+            count.incrementAndGet();
+            return true;
+        }
+        return false;
+    });
+
+    int removed = count.get();
+    if (removed > 0) {
+        logger.info("Invalidated {} hunspell dictionary cache entries for package: {}", removed, packageId);
+    } else {
+        logger.debug("No cached dictionaries found for package: {}", packageId);
+    }
+    return removed;
 }
Suggestion importance[1-10]: 6

__

Why: The sizeBefore - dictionaries.size() calculation is not atomic on a ConcurrentHashMap and could produce incorrect or negative counts under concurrent modifications. Using an AtomicInteger within the removeIf predicate provides an accurate count of actually removed entries, which is a meaningful improvement for correctness.

Low
Suggestions up to commit 4f3ab0f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Missing super.writeTo call in serialization

The writeTo method does not call super.writeTo(out), which is required to properly
serialize the parent ActionResponse state. This will cause deserialization failures
when the response is transported across nodes.

server/src/main/java/org/opensearch/action/admin/indices/cache/hunspell/HunspellCacheInvalidateResponse.java [50-56]

 @Override
 public void writeTo(StreamOutput out) throws IOException {
+    super.writeTo(out);
     out.writeBoolean(acknowledged);
     out.writeInt(invalidatedCount);
     out.writeOptionalString(packageId);
     out.writeOptionalString(locale);
     out.writeOptionalString(cacheKey);
 }
Suggestion importance[1-10]: 8

__

Why: The writeTo method in HunspellCacheInvalidateResponse is missing super.writeTo(out), which is required to properly serialize the parent ActionResponse state. This is a real bug that could cause deserialization failures when the response is transported across nodes. The corresponding HunspellCacheInfoResponse.writeTo also has the same issue (line 58-64 in that file).

Medium
Null service causes runtime injection failure

When hunspellService is null, the TransportHunspellCacheInfoAction and
TransportHunspellCacheInvalidateAction classes are still registered as action
handlers but their HunspellService dependency will not be bound, causing Guice
injection failures at runtime when those actions are invoked. Either throw an
exception or provide a no-op binding to prevent silent runtime failures.

server/src/main/java/org/opensearch/action/admin/indices/ActionModule.java [1131-1135]

 if (hunspellService != null) {
     bind(HunspellService.class).toInstance(hunspellService);
 } else {
-    logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
+    throw new IllegalStateException("HunspellService is required for hunspell cache management APIs but was not provided");
 }
Suggestion importance[1-10]: 7

__

Why: When hunspellService is null, the transport actions are still registered but their Guice dependency won't be bound, causing runtime injection failures. The suggestion to throw an IllegalStateException instead of silently logging a warning is a valid improvement that prevents confusing runtime errors. The code comment itself says "HunspellService is always available since it's created by AnalysisModule during node bootstrap", making the null case truly exceptional.

Medium
Non-atomic count calculation under concurrent access

The count calculation using sizeBefore - dictionaries.size() is not atomic on a
ConcurrentHashMap and can produce incorrect results if concurrent modifications
occur between the two size reads. Instead, count the removed entries directly within
the removeIf predicate using an AtomicInteger.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [475-492]

 public int invalidateDictionariesByPackage(String packageId) {
     if (Strings.isNullOrEmpty(packageId)) {
         logger.warn("Cannot invalidate dictionaries: packageId is null or empty");
         return 0;
     }
 
     String prefix = packageId + CACHE_KEY_SEPARATOR;
-    int sizeBefore = dictionaries.size();
-    dictionaries.keySet().removeIf(key -> key.startsWith(prefix));
-    int count = sizeBefore - dictionaries.size();
-    ...
+    java.util.concurrent.atomic.AtomicInteger count = new java.util.concurrent.atomic.AtomicInteger(0);
+    dictionaries.keySet().removeIf(key -> {
+        if (key.startsWith(prefix)) {
+            count.incrementAndGet();
+            return true;
+        }
+        return false;
+    });
+    int removed = count.get();
+
+    if (removed > 0) {
+        logger.info("Invalidated {} hunspell dictionary cache entries for package: {}", removed, packageId);
+    } else {
+        logger.debug("No cached dictionaries found for package: {}", packageId);
+    }
+    return removed;
 }
Suggestion importance[1-10]: 5

__

Why: The count calculation using sizeBefore - dictionaries.size() is indeed non-atomic on a ConcurrentHashMap, and the improved code using AtomicInteger within removeIf is a valid fix. However, the existing code's Javadoc already acknowledges this as "approximate" behavior, so the impact is limited to diagnostic accuracy rather than correctness.

Low
Suggestions up to commit b7ec9d0
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fail fast when required service dependency is null

When hunspellService is null, the HunspellService binding is skipped, which will
cause Guice injection to fail at runtime when TransportHunspellCacheInfoAction or
TransportHunspellCacheInvalidateAction are instantiated. This will result in a
hard-to-diagnose ProvisionException rather than a clear error. Either throw an
exception here or bind a no-op implementation.

server/src/main/java/org/opensearch/action/ActionModule.java [1131-1135]

 if (hunspellService != null) {
     bind(HunspellService.class).toInstance(hunspellService);
 } else {
-    logger.warn("HunspellService is null; hunspell cache management APIs will not be functional");
+    throw new IllegalStateException("HunspellService is null; cannot initialize hunspell cache management APIs");
 }
Suggestion importance[1-10]: 7

__

Why: The null check with a warning log is problematic because it will cause a ProvisionException at runtime when the transport actions try to inject HunspellService. Failing fast with a clear exception is better than a silent binding skip that leads to confusing injection failures later.

Medium
Restore correct deduplication logic for tail log messages

The original code used errorsAndWarnings.containsKey(normalizeLogLine(message)) to
avoid printing messages already shown in the errors/warnings section. The new code
filters by ERROR/WARN string presence instead, which is not equivalent — it will
omit non-error/warning messages that happen to contain those strings, and may still
print messages that were already shown in the errors section if they don't contain
those keywords.

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java [1060-1062]

 ring.stream()
-    .filter(message -> !message.contains("ERROR") && !message.contains("WARN"))
+    .filter(message -> {
+        String normalized = normalizeLogLine(message);
+        return errorsAndWarnings.keySet().stream().noneMatch(lm -> lm.normalizedMessage().equals(normalized));
+    })
     .forEach(message -> LOGGER.lifecycle("» " + message.replace("\n", "\n»  ")));
Suggestion importance[1-10]: 7

__

Why: The new filtering logic using !message.contains("ERROR") && !message.contains("WARN") is not equivalent to the original errorsAndWarnings.containsKey(normalizeLogLine(message)) check. The original correctly avoided reprinting messages already shown in the errors/warnings section, while the new code may both miss some duplicates and incorrectly filter non-error messages containing those strings.

Medium
Guard against division by zero for virtual shards

When numVirtualShards is 0, Math.floorMod(hash, 0) will throw an ArithmeticException
(division by zero). A guard check should be added to ensure numVirtualShards is
positive before performing the modulo operation.

server/src/main/java/org/opensearch/cluster/routing/OperationRouting.java [525-530]

 int numVirtualShards = indexMetadata.getNumberOfVirtualShards();
-if (numVirtualShards != -1) {
+if (numVirtualShards > 0) {
     final int hash = Murmur3HashFunction.hash(effectiveRouting) + partitionOffset;
     int vShardId = Math.floorMod(hash, numVirtualShards);
     return VirtualShardRoutingHelper.resolvePhysicalShardId(indexMetadata, vShardId);
 }
Suggestion importance[1-10]: 7

__

Why: If getNumberOfVirtualShards() can return 0, Math.floorMod(hash, 0) would throw an ArithmeticException. Changing != -1 to > 0 is a simple and correct fix that prevents a potential runtime crash. The improved_code accurately reflects the change.

Medium
Avoid caching failures via computeIfAbsent

computeIfAbsent does not handle exceptions thrown by the mapping function in a safe
way for ConcurrentHashMap — if the function throws, the key is not inserted, but a
partially-computed state may remain in some JVM versions. More critically, if
loadDictionaryFromPackage throws a checked exception wrapped in
IllegalStateException, the cache will not store the failure, causing repeated disk
I/O on every call for a missing package. Consider catching the exception outside
computeIfAbsent and only caching successful results, or explicitly checking
existence before computing.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [180-191]

-return dictionaries.computeIfAbsent(cacheKey, (key) -> {
-    try {
-        return loadDictionaryFromPackage(packageId, locale);
-    } catch (Exception e) {
+Dictionary cached = dictionaries.get(cacheKey);
+if (cached != null) {
+    return cached;
+}
+try {
+    Dictionary loaded = loadDictionaryFromPackage(packageId, locale);
+    dictionaries.put(cacheKey, loaded);
+    return loaded;
+} catch (Exception e) {
+    throw new IllegalStateException(
+        String.format(Locale.ROOT, "Failed to load hunspell dictionary for package [%s] locale [%s]", packageId, locale),
+        e
+    );
+}
 
-        throw new IllegalStateException(
-            String.format(Locale.ROOT, "Failed to load hunspell dictionary for package [%s] locale [%s]", packageId, locale),
-            e
-        );
-    }
-});
-
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that computeIfAbsent on ConcurrentHashMap won't cache failures, causing repeated disk I/O on every call for a missing package. However, the improved code introduces a race condition by using a non-atomic get-then-put pattern, which is arguably worse than the original. The concern is valid but the fix is imperfect.

Low
General
Fix inaccurate concurrent invalidation count

The count calculation using sizeBefore - dictionaries.size() is not atomic on a
ConcurrentHashMap — concurrent insertions or removals between the size() call and
the removeIf completion can produce a negative or incorrect count. While the Javadoc
acknowledges this, the returned value could be negative (e.g., -1), which callers
may not expect. Consider using an AtomicInteger counter incremented inside the
removeIf predicate for an accurate count.

[server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [475-492]](https://github.com/opensearch-project/OpenSearch/pull/20841/files#diff-7cabcd152e9c42550280358bc68ebd3effd5e148a25552bc8d710ed3...

andrross and others added 7 commits March 16, 2026 00:21
I've fixed the doc to reflect the JDK 21 minimum requirement. I've also
removed a few references to moving targets (i.e. which JDK is bundled)
since that will frequently change and this doc will be wrong again.

Signed-off-by: Andrew Ross <andrross@amazon.com>
Race condition between request completion and task resource tracking
cleanup.

The sequence of events:
1. Task is cancelled via `CancelTasksRequest`
2. The node operation throws `TaskCancelledException`
3. The response is sent back to the caller, which counts down
   `requestCompleteLatch`
4. The test's main thread wakes up from `requestCompleteLatch.await()`
   and asserts `resourceTasks.size() == 0`
5. Meanwhile, `TaskResourceTrackingService.stopTracking()` (which
   calls `resourceAwareTasks.remove()`) is invoked asynchronously
   via a `resourceTrackingCompletionListener` registered in
   `TaskManager.register()`

Steps 4 and 5 race. I was able to reproduce the failure locally using
`stess-ng` and verify this fix.

Signed-off-by: Andrew Ross <andrross@amazon.com>
RemoteShardsBalancer.balance() only rebalances by primary shard
count, not total shard count. In tight-capacity scenarios where
cluster-wide shard limits leave minimal spare slots, this prevents
the balancer from redistributing replicas to free space on other
nodes, leaving assignable shards unassigned. I believe this is the
cause of the flakiness in ShardsLimitAllocationDeciderIT.

This change introduces a unit test that deliberately targets the
tightly packed scenario. The RemoteShardsBalancer variant of the
new test will reliably fail if run repeated, though there is still
some non-determinism. Regardless, it is muted along with the exiting
integration test. If/when we improve the intelligence of
RemoteShardsBalancer then we can unmute these tests. For now the tests
exist to document this known limitation.

Signed-off-by: Andrew Ross <andrross@amazon.com>
…ct#20673)

Previously the code to capture and dedup error and warn messages from
test nodes would remove the timestamp from the message. This made
debugging timing issues difficult. For example, the message would look
like:

```
» WARN ][o.o.d.FileBasedSeedHostsProvider] [v2.19.5-0] expected, but did not find, a dynamic hosts list at [/home/ubuntu/workplace/opensearch-project/OpenSearch/qa/rolling-upgrade/build/testclusters/v2.19.5-0/config/unicast_hosts.txt]
»   ↑ repeated 2 times ↑
```

With this change it looks like:

```
» [2026-02-18T20:04:29,167][WARN ][o.o.d.FileBasedSeedHostsProvider] [v2.19.5-0] expected, but did not find, a dynamic hosts list at [/home/ubuntu/workplace/opensearch-project/OpenSearch/qa/rolling-upgrade/build/testclusters/v2.19.5-0/config/unicast_hosts.txt]
»   ↑ repeated 2 times ↑
```

Signed-off-by: Andrew Ross <andrross@amazon.com>
* Upgrade to Gradle 9.4

Also replace the eager cross-project task reference to
:libs:agent-sm:agent with Gradle-idiomatic patterns:

- Add a consumable agentDist configuration in the agent
  project that publishes the prepareAgent output directory
  as an artifact with a Category attribute.
- Add a matching resolvable agent configuration in the
  distribution subprojects to consume it via normal
  dependency resolution.
- Replace direct task references (project.prepareAgent,
  project.jar) with lazy alternatives: tasks.named() for
  task providers, lazy GStrings for deferred path
  resolution, and closure-based dependsOn.

This removes the need for an evaluationDependsOn call which forced
the agent project to be configured before the distribution
project, violating Gradle best practices around project
isolation and configuration-time coupling.

Signed-off-by: Andrew Ross <andrross@amazon.com>

* Refactor the agent wiring to use configurations

Signed-off-by: Andriy Redko <drreta@gmail.com>

* Use .singleFile and lazy evaluation for agentJar

Signed-off-by: Andrew Ross <andrross@amazon.com>

---------

Signed-off-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: Andriy Redko <drreta@gmail.com>
Co-authored-by: Andriy Redko <drreta@gmail.com>
…earch-project#20745)

* Add Virtual Shards routing and mapping overrides

Implements the initial routing foundation for Virtual Shards.

Adds settings validation for virtual shards and routing overrides via IndexMetadata custom data.

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

* Add Virtual Shards routing and mapping overrides

This PR adds the initial routing and metadata groundwork for virtual shards.

When enabled, routing now uses a virtual shard id (vShardId) before resolving to a physical shard. This separates hash-space size from physical shard count and prepares the path for future shard movement workflows.

What’s included

- New static index setting: index.number_of_virtual_shards (default -1, disabled).
- Routing update in OperationRouting.generateShardId:
    - virtual-shard path when enabled,
    - existing behavior unchanged when disabled.
- New VirtualShardRoutingHelper for vShardId -> physical shard resolution.
- Optional per-index override support via virtual_shards_routing custom metadata.
- Test coverage for:
    - enabled/disabled routing behavior,
    - validation rules,
    - override and fallback behavior.

Out of scope

- Side-car segment extraction flow.
- Transport/state-orchestration for managing override lifecycle.

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

* Add Virtual Shards routing and mapping overrides

This PR adds the initial routing and metadata groundwork for virtual shards.

When enabled, routing now uses a virtual shard id (vShardId) before resolving to a physical shard. This separates hash-space size from physical shard count and prepares the path for future shard movement workflows.

What’s included

- New static index setting: index.number_of_virtual_shards (default -1, disabled).
- Routing update in OperationRouting.generateShardId:
    - virtual-shard path when enabled,
    - existing behavior unchanged when disabled.
- New VirtualShardRoutingHelper for vShardId -> physical shard resolution.
- Optional per-index override support via virtual_shards_routing custom metadata.
- Test coverage for:
    - enabled/disabled routing behavior,
    - validation rules,
    - override and fallback behavior.

Out of scope

- Side-car segment extraction flow.
- Transport/state-orchestration for managing override lifecycle.

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

* Refactor Virtual Shards to use range-based routing

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

* Add fail-safe to VirtualShardRoutingHelper and valid configuration tracking test

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

* Fixed review comments and move to range based sharding

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

---------

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>
When a patch release (e.g., 2.19.5) is published and the release branch
is bumped to the next patch (2.19.6), BWC tests on main fail because
Version.java still references the old patch version.  This causes all
in-flight PRs to fail until Version.java is updated.

This change relaxes the logic so that BWC tests will still pass if the
checked out code uses a patch version one greater than expected. This
prevents CI failures every time a release branch increments its patch
version, but still prevents the main branch from drifting by more than
one patch version.

Signed-off-by: Andrew Ross <andrross@amazon.com>
@shayush622 shayush622 force-pushed the feat/hunspell-cache-api branch from a1a8a58 to 27502dd Compare March 16, 2026 07:25
@github-actions
Copy link
Contributor

Persistent review updated to latest commit 27502dd

@shayush622 shayush622 force-pushed the feat/hunspell-cache-api branch from 27502dd to b7ec9d0 Compare March 16, 2026 07:47
@github-actions
Copy link
Contributor

Persistent review updated to latest commit b7ec9d0

@github-actions
Copy link
Contributor

Persistent review updated to latest commit 4f3ab0f

@github-actions
Copy link
Contributor

❌ Gradle check result for 4f3ab0f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

Persistent review updated to latest commit 04f3e4b

@shayush622 shayush622 force-pushed the feat/hunspell-cache-api branch from 04f3e4b to da51569 Compare March 16, 2026 09:13
@github-actions
Copy link
Contributor

Persistent review updated to latest commit da51569

@github-actions
Copy link
Contributor

❌ Gradle check result for da51569: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…ect#20697)

* initial commit of extensible query engine plugin to sandbox

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* clean up build.gradle - update forbidden-dependencies to skip guava check in sandbox plugins,
calcite requires this dependency at compile time

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Rename plugin interfaces and default implementations.
Wire up a ppl front-end using UnifiedQueryAPI from sql plugin.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* refactor to plugin-plugin SPI

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add readmes and start some clean up.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* analyzer errors

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* move fe plugin into analytics plugin for testing only,  we will use sql plugin.
also remove "hub" plugin.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* spotless

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* clean up

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* more clean up

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fixing analyzer issues

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix javadoc

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix guava forbidden check

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix license check

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix javadoc warning on transitive dependency.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* clean up build.gradle and fix weird javadoc issues with dependencies.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix calcite/guava dependencies

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix package name

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* remove EngineCapabilities, just use calcite's sqloperatortable.
wraps this and schema in an engineContext provided to front-ends

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* simplify unified IT to use params

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix guava NOTICE file to exactly match the file from grpc

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* javadoc fix

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Update sandbox/plugins/analytics-engine/README.md

Co-authored-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: Marc Handalian <handalm@amazon.com>

---------

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Signed-off-by: Marc Handalian <handalm@amazon.com>
Co-authored-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: shayush622 <ayush5267@gmail.com>
@shayush622 shayush622 force-pushed the feat/hunspell-cache-api branch from da51569 to 6a31779 Compare March 16, 2026 11:31
@github-actions
Copy link
Contributor

Persistent review updated to latest commit 13ba03f

@github-actions
Copy link
Contributor

❌ Gradle check result for 13ba03f: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

Persistent review updated to latest commit 99f2309

@github-actions
Copy link
Contributor

❌ Gradle check result for 99f2309: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@shayush622
Copy link
Contributor Author

Closing this PR. Cache management REST API endpoints are deferred per maintainer's feedback.

Key takeaways from design discussion with @cwperks :

  1. Node-level cache — Index-level caching would lead to redundant Dictionary Objects in memory. Node-level cache allows Dictionary reuse across indices sharing the same package+locale.

  2. Reuse _reload_search_analyzers — Rather than introducing new cache invalidation endpoints, Craig recommends reusing the existing _reload_search_analyzers API. Each invocation should reload analyzers for every index by re-reading dictionary data from disk and repopulating the in-memory cache. This is considered acceptable given the operation is infrequent (essentially a one-off event).

  3. New endpoint seems premature optimization — Craig is open to a dedicated cache invalidation endpoint if needed, but considers it potential over-engineering at this stage. Will revisit if usage data shows _reload_search_analyzers is called frequently enough to warrant fine-grained cache control.

The _reload_search_analyzers integration will be addressed in a follow-up PR after Phase 1 (#20840) is merged. See RFC: #20712

@shayush622 shayush622 closed this Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants